Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix deadlock when closing pipe #324

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix deadlock when closing pipe #324

wants to merge 1 commit into from
+44 −2

Conversation

mat007
Copy link

@mat007 mat007 commented Dec 12, 2024

Hi!

This PR fixes a deadlock that we face every once in a while.

Thanks!

@mat007 mat007 requested a review from a team as a code owner December 12, 2024 16:46
@mat007 mat007 changed the title Fix race when closing pipe Fix deadlock when closing pipe Dec 12, 2024
@mat007 mat007 mentioned this pull request Dec 12, 2024
Merged
@mat007 mat007 force-pushed the close-race branch 2 times, most recently from 90fdf20 to 70746b4 Compare December 13, 2024 06:39
@mat007 mat007 mentioned this pull request Dec 13, 2024
Merged
@mat007
Fix deadlock when closing pipe
252d76a 
Signed-off-by: Mathieu Champlon <[email protected]>
@thaJeztah
Copy link
Contributor

@mat007 I think this PR fixes #257, correct? If so, can you add a fixes xxx to the PR description (not the commit message, as that tends to get noisy 😂)

@mat007
Copy link
Author

mat007 commented Dec 13, 2024

I think this PR fixes #257, correct?

I don’t know if it fixes that one, I was investigating something else. I’m really not sure it’s related. 🤔

@thaJeztah
Copy link
Contributor

Oh, sorry; someone pointed me to that ticket as the reason for maintaining a fork, so I thought this PR was related to that

@kevpar
Copy link
Member

kevpar commented Dec 18, 2024

@mat007 can you explain what the deadlock is that you're seeing?

@mat007
Copy link
Author

mat007 commented Dec 19, 2024 •
edited
Loading

@mat007 can you explain what the deadlock is that you're seeing?

Sure!

The added test without the fix never terminates. Breaking under a debugger we see that the goroutine that called Close is blocked on

go-winio/pipe.go

Line 578 in bdc6c11

<-l.doneCh
while the ListenPipe one is waiting on

go-winio/pipe.go

Lines 462 to 463 in bdc6c11

select {
case <-l.closeCh:

The problem is that we have 2 readers for l.closeCh, with the other one being

go-winio/pipe.go

Line 447 in bdc6c11

case <-l.closeCh:
but we’re signalling it by writing a value to the channel, which can only be read by one reader.
This PR fixes this by closing the channel instead of writing to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mat007 @thaJeztah @kevpar